-
Notifications
You must be signed in to change notification settings - Fork 25.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CI] Adding continuous testing for ECS dynamic templates #97901
Conversation
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
Pinging @elastic/es-delivery (Team:Delivery) |
Pinging @elastic/es-data-management (Team:Data Management) |
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixbarny @ruflin see if you agree with my comment about not failing if we create multi-field mapping even for fields of which ECS definition does not enforce such.
For example, we define a multi-field mapping for the *.name pattern. If you look into the ECS definitions, you will find that many *.name have multi-field mappings, while many others don't. Trying to be very accurate will result with many more dynamic templates.
Seems like erring on the side of always creating a match_only_text
subfield, even for cases where ECS only defines a keyword
field for *.name
fields.
I think that's fine. While on the one hand, this creates a bit more indexing overhead than strictly required, it brings a nice consistency via the naming convention *.name
so that users know they can always do a full text search on fields ending with *.name
.
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Show resolved
Hide resolved
x-pack/plugin/core/template-resources/src/main/resources/ecs-dynamic-mappings.json
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Show resolved
Hide resolved
I'm not a fan of the ECS multi fields but I agree we should be consistent. Do we have an understand on how much the overhead on storage is? I guess hard to tell? Assuming a user would want to disable this, I assume they could overwrite it in |
I can't say anything clever about the actual overhead, only that since the default mapping for strings in Elasticsearch is multi-field, then I assume it is at least acceptable. We are only extending ECS a bit here 🙂
Exactly! And since we added |
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
@rjernst @mark-vieira would you be able to review or assign someone to review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current review comments covers anything I would have added already. I see there was the discussion around multi-fields for .name, which was indeed something annoying for me when testing this out earlier, I did create an issue a long time ago in the ECS repo, but nothing really came of it.
I don't think the concept of multi-fields itself adds overhead, but text fields itself adds alot of overhead for sure, so if we had to choose, I rather not have them be multi-field but that decision is not up to me :)
Anything else seems LGTM.
Related to work done by #97901
I've added a daily ci job with notifications to slack (#es-delivery) and email ([email protected]) |
Closes #96713
All three tests cases are covered:
subobjects: false
In addition, I added verification that all ECS multi-field definitions are covered by the ECS dynamic templates (revealing two that actually were not).
@felixbarny @ruflin see if you agree with my comment about not failing if we create multi-field mapping even for fields of which ECS definition does not enforce such.
For example, we define a multi-field mapping for the
*.name
pattern. If you look into the ECS definitions, you will find that many*.name
have multi-field mappings, while many others don't. Trying to be very accurate will result with many more dynamic templates.@P1llus I assigned you as a reviewer because I think you were waiting for this in order to start migrating some ECS mappings to rely on the builtin dynamic templates.